Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: regression in list item's extra slot #5250

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hamza221
Copy link
Contributor

☑️ Resolves

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🚧 Tasks

  • ...

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@hamza221 hamza221 added 2. developing Work in progress regression Regression of a previous working feature labels Feb 15, 2024
@hamza221
Copy link
Contributor Author

cc @GretaD @susnux @ShGKme

@GretaD GretaD mentioned this pull request Feb 15, 2024
2 tasks
@susnux
Copy link
Contributor

susnux commented Feb 15, 2024

The "after" image also looks broken, for the NcListItem I would only expect two lines of text working and the tags to be shown on the very right.

@hamza221
Copy link
Contributor Author

The "after" image also looks broken,

Yes the after is still a bit broken, the tags should be aligned with the lines
This is how it looks on nc.cloud
image

for the NcListItem I would only expect two lines of text working and the tags to be shown on the very right.

Isn't the extra slot supposed to show elements below the lines

@GretaD
Copy link
Contributor

GretaD commented Feb 15, 2024

The "after" image also looks broken,

Yes the after is still a bit broken, the tags should be aligned with the lines This is how it looks on nc.cloud

this is fixed on mail side, i already pushed a pr for that nextcloud/mail#9336

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes reverting a11y fixes and are not valid -> must not be done. You have to find a solution with the actions outside the anchor element

@@ -394,21 +394,20 @@
</div>
</div>
</div>
<!-- Actions -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must not be done! An interactive element (the a) must not contain any other interactive element (the NcActions).
We changed this just a couple of weeks ago.

@susnux
Copy link
Contributor

susnux commented Feb 16, 2024

Was there a design review? It seems this component gets extended to an all-in-one-solution.
Especially that there is now a 3rd and 4th line introduced, which might be ok but we need to make sure not to break anything (e.g. a11y where you need to make sure to NOT include interactive elements ).

@hamza221
Copy link
Contributor Author

Makes more sense now thank you for the explanation, I'll try to work on a different solution

@susnux
Copy link
Contributor

susnux commented Feb 18, 2024

I mean if it fits in the component fine, but we can not have nested interactive elements. This only works with some specific focus handling (e.g. how menu bars work, you can tab the first element but next tab will be outside the menu bar, you then need to use arrows to change focus).

@ShGKme ShGKme added this to the 8.8.2 milestone Feb 29, 2024
@juliusknorr juliusknorr modified the milestones: 8.8.2, 8.9.1 Mar 6, 2024
@Pytal Pytal modified the milestones: 8.9.1, 8.9.2 Mar 6, 2024
@ShGKme ShGKme modified the milestones: 8.9.2, 8.10.0, 8.10.1 Mar 8, 2024
@ShGKme ShGKme modified the milestones: 8.10.1, 8.11.0, 8.11.1 Mar 15, 2024
@Antreesy Antreesy modified the milestones: 8.11.1, 8.11.2 Mar 21, 2024
@Antreesy Antreesy modified the milestones: 8.11.2, 8.11.3 Apr 11, 2024
@Antreesy Antreesy removed this from the 8.11.3 milestone May 7, 2024
@Antreesy Antreesy added this to the 8.11.4 milestone May 7, 2024
@susnux susnux modified the milestones: 8.11.4, 8.12.1 May 19, 2024
@GretaD
Copy link
Contributor

GretaD commented Jun 5, 2024

Hello @susnux, this is still an issue on mail, i would get @jancborchardt @marcoambrosini @nimishavijay involved, what would be a good compromise :)

If it was up to me, I would keep it on the right side, with the argument, that its like this since couple of months and nobody complained about the space, which means that either people are fine with it, or its not used at all :), now that we have different layouts, people can always choose a layout where the list is wide. But, of course, i would be fine to work on another solution.

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Jun 5, 2024

An interactive element (the a) must not contain any other interactive element (the NcActions).

Are you sure that this is the case @susnux ? afaik an <a> cannot contain another <a> but it can contain buttons

e.g. most cards in card layouts
Screenshot 2024-06-05 at 17 34 58

@ShGKme
Copy link
Contributor

ShGKme commented Jun 5, 2024

e.g. most cards in card layouts

In valid implementation, they are not wrapped in <a>. They have a link inside and either use custom onclick (like we do in Files) or stretch the link in a pseudo element (like we do in Apps list).

But in both HTML validity and accessibility having nested interactive elements is not valid.

@ShGKme
Copy link
Contributor

ShGKme commented Jun 5, 2024

@GretaD
Copy link
Contributor

GretaD commented Jun 18, 2024

thank you for all the feedback,

@marcoambrosini how should we proceed then?

@marcoambrosini
Copy link
Contributor

I donn't know which would be the better approach. cc @ShGKme and @susnux

@susnux susnux modified the milestones: 8.12.1, 8.13.1, 8.14.0 Jun 25, 2024
@susnux susnux modified the milestones: 8.14.0, 8.14.1 Jul 4, 2024
@hamza221
Copy link
Contributor Author

what we're trying to accomplish is just a third line for 'non interactive- non clickable "tags" ' would that be acceptable ?
So move the slot outside the NcActions rename it (thirdline? ) ?

@susnux susnux modified the milestones: 8.14.1, 8.15.0 Jul 22, 2024
@Antreesy Antreesy modified the milestones: 8.15.0, 8.15.1 Jul 23, 2024
@susnux susnux modified the milestones: 8.15.1, 8.15.2 Jul 29, 2024
@ShGKme ShGKme modified the milestones: 8.15.2, 8.16.0 Aug 3, 2024
@Antreesy Antreesy modified the milestones: 8.16.0, 8.16.1 Aug 5, 2024
@susnux susnux modified the milestones: 8.16.1, 8.17.0 Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Envelope layout is broken
8 participants